-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display of Annotation Type #1192
Conversation
5ada9a5
to
2712d54
Compare
First of all please fix codacy and build (probably you forgot to add migrations). Please check that the patch works in case of undo/redo, propagation, ... When you ready please remove WIP from the patch and we will review and provide feedback. |
2712d54
to
0ae7df8
Compare
0ae7df8
to
f0303cf
Compare
@azhavoro , could you please review and give your opinion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution.
I have question:
What do you think about support of this field for export/import annotations functionality (CVAT format only). For now if I dump and upload anno again I will completely lose auto/manual field for all objects, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Priya4607 , we have a conflict. Could you please resolve it?
@azhavoro , could you please summarise all your comments into a check list? Thus it will be easy to understand where we are with the PR.
e985dc5
to
da8726b
Compare
@Priya4607 thank you for the update
Could you please fix the build and squash all migrations into one file? |
da8726b
to
5c170c8
Compare
@Priya4607 , what is your plans regarding the PR? |
@nmanovic |
I'm ok with the back-end changes (2 minor comments above). |
*added field type for tracks
@Priya4607 , could you please look at #1767. Do you think you can adapt your changes to the PR? It looks like I will merge it before your PR. I hope that together we can merge your PR till middle of July. Really appreciate your time and efforts! |
@azhavoro , please work with @Priya4607 to accept the PR. What should be done for that? Could you please preview it again and provide your comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Priya4607 thanks for the PR again!
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but few recently added tests in cvat/apps/dataset_manager/tests/test_annotation.py
should be updated.
migrations.AlterField( | ||
model_name='labeledshape', | ||
name='type', | ||
field=models.CharField(choices=[('rectangle', 'RECTANGLE'), ('polygon', 'POLYGON'), ('polyline', 'POLYLINE'), ('points', 'POINTS'), ('cuboid', 'CUBOID')], max_length=16), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have the migration for "type" field here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a migration conflict. So I merged migrations to resolve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Priya4607 , we should have the old migration file and plus new one with only your changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I create a new migration, I get the labeledshape
model changes added. I'm not sure how to include my changes alone. Could you provide some suggestions on how to proceed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Priya4607 , it was a problem with develp branch. It missed some migrations. I have fixed that. Could you please update and create a new migration?
cvat/apps/tf_annotation/views.py
Outdated
@@ -174,6 +174,7 @@ def convert_to_cvat_format(data): | |||
"group": None, | |||
"occluded": False, | |||
"attributes": [], | |||
"annotation_type": "Auto", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Priya4607 , can we rename "annotation_type" into "source". Also I will vote for "auto", "manual" (fist letter should be lower case). In the future we can store in the field even username (or its id).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, should it be applied to both frontend and backend to maintain uniformity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so.
@Priya4607 , we are ready to merge the PR. Let's do the final minor refactoring, update CHANGELOG.md and I will merge. Thanks for the great idea and contribution. |
@zhiltsov-max Updated test cases |
CHANGELOG.md
Outdated
@@ -61,6 +61,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
- [Datumaro] Added `convert` command to convert datasets directly (<https://github.com/opencv/cvat/pull/1837>) | |||
- [Datumaro] Added an option to specify image extension when exporting datasets (<https://github.com/opencv/cvat/pull/1799>) | |||
- [Datumaro] Added image copying when exporting datasets, if possible (<https://github.com/opencv/cvat/pull/1799>) | |||
- Source type support for tags, shapes and tracks (<https://github.com/opencv/cvat/pull/1192>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong place. Could you please move these lines into the section for next release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
@Priya4607 , I'm fine with the PR. Need to fix CHANGELOG.md and I will merge. Thanks! |
Here is an overview of what got changed by this pull request: Complexity increasing per file
==============================
- cvat/apps/engine/migrations/0027_auto_20200719_1552.py 1
- cvat-core/src/annotations-objects.js 1
Clones removed
==============
+ cvat-core/src/annotations-saver.js -1
See the complete overview on Codacy |
CHANGELOG.md
Outdated
@@ -467,6 +468,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Added | |||
- Initial version | |||
|
|||
## [0.6.1] - 2020-04-14 | |||
### Added | |||
- Annotation type to determine whether a drawn shape is auto annotated using Tensorflow Object detection API or manually annotated by the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Priya4607 , some problems with merge. These lines are wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed those lines.
@Priya4607 , thanks again for the PR. If you can add the information into statistics about the task (aka info), it will be awesome. Thus we can understand number of manual, interpolated and automatic annotated objects. |
This PR adds a feature which can help the user distinguish which of the boxes are auto-annotated. When a user annotates an image it is marked as
(MANUAL)
in each of the boxes created and in case of auto-annotation it is marked as(AUTO)
. When the user edits a auto-generated box it is changed from(AUTO)
to(MANUAL)
.